Show all revisions when run `service describe -v` #790

MIBc · 2020-04-08T12:23:42Z

Fixes: ##789

The kn service describe -v command shows repetitive revisions, because
the revision would be covered by next one.

knative-prow-robot · 2020-04-08T12:23:57Z

Hi @MIBc. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

navidshaikh · 2020-04-08T12:30:04Z

/ok-to-test

rhuss

Thanks for the PR, good find !

I wonder however if we could just change the signature to pass by-value not by-reference, so that a deep copy would not be necessary. Tbh, I don't yet understand why (and if) the called method modifies the revision

rhuss · 2020-04-08T14:50:57Z

pkg/kn/commands/service/describe.go

@@ -321,7 +321,7 @@ func completeWithUntargetedRevisions(client clientservingv1.KnServingClient, ser
 			continue
 		}
 		revisionsSeen.Insert(revision.Name)
-		newDesc, err := newRevisionDesc(&revision, nil, service)
+		newDesc, err := newRevisionDesc(revision.DeepCopy(), nil, service)


Couldn't we just past the revision by calue, not by reference ? I think this would be even cleaner. wdyt ?

That will modify the parameter type in newRevisionDesc function. It brings some changes which need to test.

maximilien

Minor comment. Otherwise LGTM

maximilien · 2020-04-09T00:50:30Z

pkg/kn/commands/service/describe_test.go

@@ -432,28 +432,31 @@ func TestServiceDescribeVerbose(t *testing.T) {
 	r := client.Recorder()

 	// Prepare service
-	expectedService := createTestService("foo", []string{"rev1", "rev2"}, goodConditions())
+	expectedService := createTestService("foo", []string{"rev1", "rev2", "rev3"}, goodConditions())


Do we have a test for the base case with one revision? I think there is but worth double checking. Thx

We have the base case with one revision.

MIBc · 2020-04-09T15:44:16Z

I wonder however if we could just change the signature to pass by-value not by-reference, so that a deep copy would not be necessary. Tbh, I don't yet understand why (and if) the called method modifies the revision

I found this code would cover the revision. But not create a new revision variable.

rhuss · 2020-04-09T16:39:31Z

I found this code would cover the revision. But not create a new revision variable.

I think I understand (excellent find by the way!), but if we don't move the pointer but the value to the function for extracting the revision description, this should be similar to a copy, or ? (not a deep copy, but I think a shallow copy should be good enough, too).

So changing that function signature from taking a point to taking a value should be good enough, i think.

* The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one.

knative-metrics-robot · 2020-04-10T06:45:54Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/describe.go	79.8%	82.2%	2.5

MIBc · 2020-04-10T07:06:26Z

The reason we don't use deep copy is that it cost more time. Right?

MIBc · 2020-04-10T07:44:01Z

/retest

MIBc · 2020-04-11T05:41:10Z

/retest

rhuss · 2020-04-14T08:40:49Z

The reason we don't use deep copy is that it cost more time. Right?

I'm not really worried about costs, it's just that I think its cleaner and simpler, but I'm not dogmatic. If you think a deep copy has other advantages or is more idiomatic, I'm happy, too ;)

rhuss

thanks !

/lgtm

knative-prow-robot · 2020-04-14T08:41:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, MIBc, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one.

* (refactor) address the e2e extract / refactor of issue #763 (#765) * (refactor) address the e2e extract / refactor of issue #763 * various updates to address reviewers feedback * renamed lib/test/integration to lib/test and package to test Signed-off-by: Roland Huß <[email protected]> # Conflicts: # CHANGELOG.adoc # test/e2e/service_export_import_apply_test.go # test/e2e/trigger_test.go * fix(plugin): Fix plugin lookup with file ext on Windows (#774) * fix(plugin): Fix plugin lookup with file ext on Windows * chore: Update changelog * fix: Reflect review feedback * fix: Reflect review feedback and add future todo Signed-off-by: Roland Huß <[email protected]> # Conflicts: # CHANGELOG.adoc * fix(issue #762): correct error message when updating service (#778) * fix(issue #762): correct error message when updating service * correct message when updating service and passing many names * fix issue with TestServiceUpdateWithMultipleImages running create vs update * * added TestServiceDescribeWithMultipleNames * added TestServiceCreateWithMultipleNames * fix error message for service delete since many names can be passed * Use vendored deps while running e2e locally (#783) Also set GO111MODULE=on unconditionally * Update sink binding create usage string (#785) * Add "--target-utilization" to manage "autoscaling.knative.dev/targetUtilizationPercentage" annotation (#788) * Support setting "autoscaling.knative.dev/targetUtilizationPercentage" annotation. Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/service_options_test.go * Remove the delete propagation flag (#770) * Remove the delete propagation flag In it's current state it now takes me about 25 seconds for the `kn delete` to complete. Before #682 it used to be almost immediate. This is because we now pass in the `DeletePropagationBackground` flag. I believe this is a mistake, not only because of the 20+ seconds of additional time to delete things, but IMO the CLI should talk to the server in the same way regardless of the --wait flag. That flag should just be a CLI thing to indicate if the user wants the CLI to wait for the server to complete but not HOW the server should do the delete. Signed-off-by: Doug Davis <[email protected]> * try just tweaking the --no-wait flag Signed-off-by: Doug Davis <[email protected]> * Fix error when output is set to name (#775) * fix error when output is set to name * add e2e test * change to flags/listprint.go Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/basic_workflow_test.go * Show all revisions when run `service describe -v` (#790) * The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one. * Fix resource listing with -oname flag (#799) * Fix resource listing with -oname flag * add e2e tests Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/ping_test.go # test/e2e/revision_test.go # test/e2e/route_test.go # test/e2e/source_apiserver_test.go # test/e2e/source_binding_test.go # test/e2e/trigger_test.go * Make wait, no-wait and async flags per bool var CLI convention (#802) * Make wait, no-wait and async flags per bool var CLI convention Fixes #800 - Deprecated bool vars can be supported for CLI convention - Bind --async flag value to --no-wait - Only one flag among [wait, no-wait, async] can be provided, else raise an error * Simplify conditionals * Add unit tests for deprecated flag async * Fix a typo * e2e: Foreground delete for revisions and services in e2e (#794) * e2e: Foreground delete for revisions and services in e2e to avoid any race conditions and flakes * Use --wait instead of --no-wait=false Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/basic_workflow_test.go # test/e2e/revision_test.go * e2e: Run tekton e2e against pipeline v0.11.1 (#803) * Use buildah task from master branch and paramterize FORMAT * Configure pipeline v0.11.1 * DNM: Run tekton e2e in this PR * Revert "DNM: Run tekton e2e in this PR" This reverts commit 903f5be. * Update CHANGELOG for v0.13.2 (#804) * Pin serving to v0.13.2 and update version command (#797) * Pin serving v0.13.2 dep to v0.13.2 * Update version command now points to serving v0.13.2 and eventing v0.13.6 * Copy go.sum as generated in CI Signed-off-by: Roland Huß <[email protected]> # Conflicts: # go.mod # go.sum # vendor/modules.txt * add missing vendored files * fixed error reporting for traffics tests * Updated test * fix formatting * e2e for service export (#739) * e2e for service export * e2e for service export * e2e for service export * e2e for service export * e2e for service export Signed-off-by: Roland Huß <[email protected]> # Conflicts: # test/e2e/service_export_import_apply_test.go Co-authored-by: dr.max <[email protected]> Co-authored-by: David Simansky <[email protected]> Co-authored-by: Navid Shaikh <[email protected]> Co-authored-by: Lv Jiawei <[email protected]> Co-authored-by: Doug Davis <[email protected]> Co-authored-by: Ying Chun Guo <[email protected]> Co-authored-by: Murugappan Chetty <[email protected]>

…ve#790) * Add directory containing resources for Devstats site * Markdown files follow mdl rules * Move to tools directory, add license header * Fix reference

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 8, 2020

knative-prow-robot requested review from maximilien and navidshaikh April 8, 2020 12:23

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 8, 2020

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 8, 2020

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 8, 2020

rhuss reviewed Apr 8, 2020

View reviewed changes

maximilien approved these changes Apr 9, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2020

MIBc force-pushed the copy branch from a2dd439 to 34076cc Compare April 10, 2020 06:39

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 10, 2020

Show all revisions when run service describe -v

1a933d6

* The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one.

MIBc force-pushed the copy branch from 34076cc to 1a933d6 Compare April 10, 2020 06:43

rhuss approved these changes Apr 14, 2020

View reviewed changes

knative-prow-robot assigned rhuss Apr 14, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2020

rhuss added the backport/candidate Consider this PR to be backported to the release branch label Apr 14, 2020

knative-prow-robot merged commit de4c36f into knative:master Apr 14, 2020

dsimansk mentioned this pull request Apr 14, 2020

service describe show repetitive revisions if service has multi revisions #789

Closed

rhuss pushed a commit to rhuss/knative-client that referenced this pull request Apr 15, 2020

Show all revisions when run service describe -v (knative#790)

6843871

* The `kn service describe -v` command shows repetitive revisions, because the revision would be covered by next one.

rhuss mentioned this pull request Apr 15, 2020

Pr/release 0.13.2 backports #806

Merged

navidshaikh added backport/pr A backport PR which is target to a release branch. and removed backport/candidate Consider this PR to be backported to the release branch labels Apr 20, 2020

rhuss added backported-to/0.13 and removed backport/pr A backport PR which is target to a release branch. labels Apr 20, 2020

MIBc mentioned this pull request Aug 28, 2020

REQUEST: New membership for MIBc knative/community#243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show all revisions when run `service describe -v` #790

Show all revisions when run `service describe -v` #790

MIBc commented Apr 8, 2020

knative-prow-robot commented Apr 8, 2020

navidshaikh commented Apr 8, 2020

rhuss left a comment

rhuss Apr 8, 2020

MIBc Apr 9, 2020

maximilien left a comment

maximilien Apr 9, 2020

MIBc Apr 9, 2020

MIBc commented Apr 9, 2020 •

edited

Loading

rhuss commented Apr 9, 2020

knative-metrics-robot commented Apr 10, 2020

MIBc commented Apr 10, 2020

MIBc commented Apr 10, 2020

MIBc commented Apr 11, 2020

rhuss commented Apr 14, 2020

rhuss left a comment

knative-prow-robot commented Apr 14, 2020

Show all revisions when run service describe -v #790

Show all revisions when run service describe -v #790

Conversation

MIBc commented Apr 8, 2020

knative-prow-robot commented Apr 8, 2020

navidshaikh commented Apr 8, 2020

rhuss left a comment

Choose a reason for hiding this comment

rhuss Apr 8, 2020

Choose a reason for hiding this comment

MIBc Apr 9, 2020

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

maximilien Apr 9, 2020

Choose a reason for hiding this comment

MIBc Apr 9, 2020

Choose a reason for hiding this comment

MIBc commented Apr 9, 2020 • edited Loading

rhuss commented Apr 9, 2020

knative-metrics-robot commented Apr 10, 2020

MIBc commented Apr 10, 2020

MIBc commented Apr 10, 2020

MIBc commented Apr 11, 2020

rhuss commented Apr 14, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Apr 14, 2020

Show all revisions when run `service describe -v` #790

Show all revisions when run `service describe -v` #790

MIBc commented Apr 9, 2020 •

edited

Loading